Rename prohibitAllSharing to containsRestrictedData - #234
Conversation
github-actions
Bot
added
workshop/frontend
c04d5e3 to
aa71300
Compare
fd04b0d to
42aefbf
Compare
cf3449d to
f633a1a
Compare
5f1ff10 to
e4e661c
Compare
f633a1a to
bc6f07a
Compare
Preview:
|
413e955 to
b2c3e58
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A mechanical rename with no behavior change, preparing to replace the all-or-nothing sharing lockdown with per-collaborator observer verification (next commit): the flag's real meaning is "this observation contains restricted data", and what the platform does about that is policy that should not be baked into the name. - `ObservationDescription.prohibitAllSharing` becomes `containsRestrictedData` (no alias). The overseer's durable singleton keeps its historical storage key -- renaming it would silently unlatch every workspace that already carries it -- with a NOTE explaining why. - `GadgetMetadata.sharingProhibited` becomes `containsRestrictedData`. - BigQuery call sites, comments, and docs follow the rename. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A sensitive observation used to be blocked whenever the workspace had any share at all, and latched a mode that blocked all future sharing. Observer verification (addObserver/getVerifier, re-checked on every open) has fully landed and is the intended replacement for the sharing half of that latch, so: - `authorizeObservation` no longer throws just because shares exist. Instead a narrower coverage guard requires every *current collaborator* to hold an observer record covering the producing gatekeeper, closing the live-session gap (verification otherwise only runs at open()). Unredeemed share links no longer block: redemption is gated at open(), where ensureObserver runs before the recipient sees anything. - The sharing enforcement sites are deleted: the open() short-circuit, the receiveExternalMessage refusal, and the addCollaborator / createShareLink / newShareLinkKey throws. - Restricted mode is unchanged: a latched workspace still cannot perform actions or fetch from the public web. - The ShareModal's blocking panel is now a non-blocking informational banner, since sharing is allowed but recipients must verify. - The integration-test fixture gains a real session (readThing/doThing) so tests can drive the observation/action policy through the real ApprovalQueue funnel; new tests cover the coverage guard and the restricted-mode latch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b2c3e58 to
1991473
Compare
receiveExternalMessage admitted any build collaborator without observer verification, so a never-verified collaborator could prompt the agent and receive replies surfacing already-observed restricted data. Pull role resolution + ensureObserver into one gate, OverseerImpl.authorizeCollaborator, and run it from both open() and receiveExternalMessage (non-interactively). Also keep local .dev.vars/.env out of the integration harness so suites behave the same locally as in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Removal on a restricted-data workspace is newly reachable (it used to be locked down before it could ever have collaborators), and the coverage guard leans on two untested behaviors: listCollaborators must exclude lazily-revoked records so removal unblocks sensitive observations, and removal must tear down the observer record so a re-added collaborator can't silently reuse stale coverage. Also remove SharingManager.hasAnyShares(), dead since the wholesale sharing block was its only caller: the one thing it adds over listCollaborators -- counting unredeemed share links -- is exactly what the new model rejects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cd22d16 to
b7c9372
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ole scope Two fixes from review on the observer-verification work: - #assertSensitiveObservationCoverage demanded coverage of every current collaborator, but ensureObserver only ever verifies a collaborator against their role's scope -- and for "use" that excludes connections no gadget binds. A "use" collaborator therefore blocked sensitive reads through an unbound connection *permanently*, and the error's remedy (re-open the workspace) could never help. The guard now skips collaborators in whose role-scope the producing gatekeeper does not fall; an unverifiable gatekeeper still blocks on any collaborator. - receiveExternalMessage ran observer verification before checking the caller's role, so a "use" collaborator was either verified for access the path can never grant (real addObserver calls plus a persisted observer record) or told to fix a verification failure that would never grant it. authorizeCollaborator now takes an optional required role, resolved before verification; the external path requires "build" and denies insufficient roles with the plain access message. The new integration tests live in their own file (own harness), because the sensitive-observations suite includes a revocation-restart abort whose local-harness blast radius makes that file's concurrent timing fragile to grow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Growing the positional tail had already forced one call site into a placeholder `undefined` argument; an options object names what each caller actually opts into. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
41ae6a0 to
740374a
Compare
|
|
||
| const isOwner = !metadata.owner | ||
| const sharingProhibited = metadata.sharingProhibited === true | ||
| const containsRestrictedData = metadata.containsRestrictedData === true |
There was a problem hiding this comment.
🟡 Kernel and UI changes are shipped in the same commits, against the repository's review rule
The change edits the kernel and the browser UI together in single commits (packages/workshop-frontend/src/ShareModal.tsx:374 alongside packages/workshop-backend/src/overseer.ts), so the kernel cannot be reviewed apart from the UI as the repository requires.
Impact: Reviewers cannot review the security-critical kernel diff independently, which the project's review policy mandates.
Rule and commit layout
REVIEW.md ("High-scrutiny areas"): "A large kernel change should be split by concern into separate PRs, and at minimum grouped into commits that let workshop-backend/workshop-shared be reviewed apart from UI. Flag PRs that bundle both."
Commits 6b778a1 and 1991473 each touch packages/workshop-backend/src/overseer.ts, packages/workshop-shared/src/api.ts, packages/workshop-shared/src/gatekeeper.ts and packages/workshop-frontend/src/ShareModal.tsx in one commit.
Prompt for agents
Per REVIEW.md, kernel changes (packages/workshop-backend, packages/workshop-shared) must be reviewable apart from UI changes. Restructure the history so the ShareModal.tsx (workshop-frontend) edits live in their own commit(s) separate from the overseer.ts/api.ts/gatekeeper.ts changes, or split the frontend change into its own PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| vendorId !== null && this.#inScopeGatekeepers("use").some(gk => gk.id === gatekeeperId); | ||
|
|
||
| for (let collaborator of collaborators) { | ||
| // A verifiable gatekeeper outside a "use" collaborator's scope is one the UI can't invoke | ||
| // and ensureObserver can't cover; only the unverifiable case above blocks regardless of | ||
| // role. An absent role means "build" (see CollaboratorInfo), which fails safe here. | ||
| if (vendorId && (collaborator.role ?? "build") === "use" && !inUseScope) continue; | ||
| let observer = vendorId ? this.storage.observers.get(collaborator.profile.id) : undefined; | ||
| if (!observer || !(gatekeeperId in observer.accountChoices)) { | ||
| throw new Error( | ||
| "This observation was blocked because it contains sensitive data, but this " + | ||
| `workspace is shared with ${collaborator.profile.name} (${collaborator.profile.id}), ` + | ||
| "who has not been verified to have access to that data. They must re-open the " + | ||
| "workspace (which verifies their access) or be removed from it before this data " + | ||
| "can be read."); | ||
| } |
There was a problem hiding this comment.
🟨 A "use" collaborator can now open a workspace holding restricted data without being verified against the connection that read it
Removing the wholesale block in open() (packages/workshop-backend/src/overseer.ts:6751-6756) means a non-owner may now open a workspace that has observed containsRestrictedData, and verification is only run for gatekeepers in their role's scope. For a use collaborator, #inScopeGatekeepers("use") (packages/workshop-backend/src/overseer.ts:6091-6112) only includes connections bound by a non-pending gadget, so a connection that read restricted data through the agent's chat bindings only (never bound to a gadget) is never verified against them, and the observation-time coverage guard explicitly skips them for that connection (packages/workshop-backend/src/overseer.ts:3002). If the agent persists that restricted data into gadget code/storage, the use collaborator renders it through the gadget UI with no access check at all — previously impossible, since a latched workspace refused every non-owner open.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comments and docs this branch introduced narrated what the code used to
do ("used to be blocked outright", "no longer a wholesale block",
"replacement for the old reliance"), which is noise once merged; state
present behavior instead. Also drop rot-prone specifics from
observers.md -- file:line references (this branch already shifted them)
and the commit hash -- keeping file and symbol names.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
authorizeObservation latched the workspace before #enforceExcludeObservers ran, so an observation blocked by the exclusion gate still left the workspace restricted even though it delivered no data. The latch now writes after both gates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#assertSensitiveObservationCoverage guarded observerVendorId for the
producing gatekeeper but then called #inScopeGatekeepers("use"), which
runs observerVendorId on every connection record -- so one legacy record
anywhere in the workspace made sensitive reads from a healthy gatekeeper
throw the legacy-reconnect error. The gadget-bound-ids computation is now
its own helper, and the coverage check reads it directly: only the
producing gatekeeper's own record decides whether it is verifiable.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
open() redeems a share key before authorizeCollaborator runs, since a brand-new recipient has no role to verify until redemption grants one. A recipient whom observer verification then refused was left persisted as a reachable collaborator, counting against sensitive-observation coverage and blocking all sensitive reads until the owner removed them -- a denial any refused link recipient could trigger by opening once. redeemShareKey now reports whether it added an edge, and open() severs exactly that grant when verification throws; the recipient can redeem the same key again once their access is fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
requireRole was an exact match, which coincides with a floor only while
its sole caller passes the maximum role ("build"): a future caller
passing "use" would silently deny build collaborators the access their
role strictly includes. Ranking the comparison (via sharing.ts's newly
exported roleRank) makes the option mean what its callers expect.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TestSession owned the queue stub dup'd in startSession() but never released it, leaking the server-side resource for the fixture's lifetime. Also drops a stray blank line in sharing.test.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // --- Collaborator management --- | ||
| // | ||
| // The sharing/permission logic lives in SharingManager (./sharing). These methods handle only | ||
| // the RPC-bound pieces (resolving profiles via User DOs, the `prohibitAllSharing` policy) and | ||
| // delegate the rest. | ||
| // the RPC-bound pieces (resolving profiles via User DOs) and delegate the rest. Note that | ||
| // sharing stays available even after the workspace observes sensitive data | ||
| // (`containsRestrictedData`): whether a given collaborator may actually see that data is | ||
| // enforced per-gatekeeper by observer verification (authorizeCollaborator at every non-owner | ||
| // entry point, and the coverage guard in authorizeObservation), not by blocking sharing | ||
| // wholesale. |
There was a problem hiding this comment.
🟥 Removing a restricted-data connection while an invite link is outstanding lets an unverified recipient see the data
The new removal guard in GatekeeperClientImpl.remove() refuses deletion only when listCollaborators().length > 0, which counts reachable collaborators but not outstanding, never-redeemed share links. Since sharing is now permitted after a workspace latches containsRestrictedData, an owner can create a share link, delete the connection whose addObserver() is the only check that could refuse a recipient, and the recipient can then redeem the link and open the workspace. ensureObserver verifies only against gatekeepers that still exist (packages/workshop-backend/src/overseer.ts:6166-6176), and #assertSensitiveObservationCoverage never runs for a deleted connection, while the restricted data persists in chat history and gadget storage.
Was this helpful? React with 👍 or 👎 to provide feedback.
removeGatekeeper() deleted the record and facet outright, but the gatekeeper record is what observer verification (and the sensitive- observation coverage guard) runs against, while the restricted data it read outlives it in chat history and storage. Removing the connection therefore let a never-verified collaborator open unchecked. remove() now refuses while the workspace is latched and has collaborators, naming the remedy (remove collaborators first). Unverifiable records -- legacy, or no vendor account behind them -- stay removable, since they anchor no verification and their removal is itself a remedy; internal callers (creation-failure rollback, ambient reconciliation) are unguarded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The blocked-observation message embedded profile.id (often an email address) and propagates to sandboxed gadget code and agent output -- an audience that cannot otherwise list collaborators. The name is the actionable part and stays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
open() rolled a redemption back when observer verification threw, but not when authorizeCollaborator returned null -- the link's creator being unreachable in the permission graph at open time. That left the refused recipient persisted as an inert collaborator who would spring back, unverified, if the creator regained access, recreating the read-block the rollback exists to prevent. Sever the grant on that path as well. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| } catch (err) { | ||
| // Verification refused the caller. If this open() had just redeemed a share key, sever | ||
| // the grant it added: a refused recipient must not persist as a reachable collaborator, | ||
| // where they would block sensitive observations until the owner removed them. They can | ||
| // redeem the same key again once their access is fixed. (The revert persists despite the | ||
| // rethrow: DO storage is not rolled back when an RPC throws.) | ||
| if (redeemedLinkId) { | ||
| sharing.revertShareKeyRedemption(profileId, redeemedLinkId); | ||
| } | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
🔴 A share-link invitee permanently loses access when their first open fails or they cancel the account prompt
The just-redeemed share-link grant is thrown away (revertShareKeyRedemption at packages/workshop-backend/src/overseer.ts:6780) on every failed open, not only on a settled refusal, so a recipient who cancels the account prompt or hits a temporary hiccup can never get in with that link again.
Impact: Invited users are locked out of the workspace after one cancelled or transient failed open and need a brand-new invitation.
Why a retry cannot recover: the key is already gone from the URL
open() redeems the share key first (packages/workshop-backend/src/overseer.ts:6748), then calls authorizeCollaborator. Any throw from that call — a cancelled ObserverConfigCallback.configure() (the frontend rejects with OBSERVER_CANCELLED, see packages/workshop-frontend/src/useWorkspaceOpen.ts:96-113), a gatekeeper outage, an expired connected account, or the "you must connect an account for every service" error — reaches the new catch and severs the shareKey edge.
The comment claims "They can redeem the same key again once their access is fixed", but the client removes the #share= fragment from the URL as soon as the open starts: onShareKeyConsumed() is invoked before openGadget() (packages/workshop-frontend/src/useWorkspaceOpen.ts:92-93) and GadgetEditor.tsx:465-467 navigates with search: {} and no hash, replace: true. load() re-reads window.location.hash, so both the in-app retry and a page reload now send shareKey === undefined. With the edge reverted, role resolution yields no role and the user gets the terminal access-denied page forever.
Before this change the redemption persisted through a failed verification, so a retry resolved the role from the stored edge and only re-ran verification.
Prompt for agents
In OverseerDurableObject.open() (packages/workshop-backend/src/overseer.ts), a share-key redemption performed earlier in the same call is now reverted whenever authorizeCollaborator() throws. That catch fires for every failure mode, including recoverable ones: the user cancelling the observer configuration modal, a gatekeeper being temporarily unreachable, or an expired connected account. Because packages/workshop-frontend/src/useWorkspaceOpen.ts calls onShareKeyConsumed() *before* openGadget() and GadgetEditor.tsx immediately rewrites the URL without the '#share=' fragment, the raw key is no longer available to the client, so neither the retry button nor a reload can redeem it again — the invitee is permanently denied. Consider either (a) not reverting on failures that are not a settled refusal, (b) keeping the share key available on the client until open() succeeds (strip the fragment only after a successful open, or remember the key in memory for retries), or (c) letting the recipient retain the redemption but relying on the observation coverage guard to block sensitive reads until they are verified. Whatever is chosen, the retry path in useWorkspaceOpen.ts must still be able to present the key.
Was this helpful? React with 👍 or 👎 to provide feedback.
remove() refused while the workspace was latched and had collaborators, but an unredeemed share link creates no collaborator state -- only a shareKeys record -- so listCollaborators() missed it. Keys are multi-redeemable and never expire, and redemption is gated at open() only while the gatekeeper record exists, so latch -> create link -> remove connection -> redeem let the recipient open verified against nothing while the restricted data persists in chat history. The guard now also refuses while any unrevoked share link is outstanding, and the error names both remedies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The frontend strips #share= from the URL (and history) before openGadget
is even issued, and kept the key nowhere. But a failed or cancelled
first open -- e.g. the invitee dismissing the observer-config modal --
reverts the redemption server-side, so every in-app retry ("Try again",
reconnection) re-read the now-empty hash and dead-ended on
access-denied, looking like a revocation. The keys stay redeemable, so
re-clicking the original link always recovered; now the hook also
retains the consumed key (per workspace id, in a ref) and falls back to
it when the hash is empty. Re-sending after a successful open is a
server-side no-op. A full reload still requires re-clicking the link.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Old model: prohibitAllSharing blocked all sharing the moment sensitive data was observed. No one other than the owner could access a latched workspace.
New model: containsRestrictedData allows sharing but requires per-collaborator observer verification against each gatekeeper that read sensitive data. The workspace still goes into a restricted mode (no actions against gatekeepers, no web fetches), but collaborators can open it if they pass verification.
WIP